Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

SAK-50696 - LTI Fix Non-Deterministic Unit Tests #13014

Merged
merged 2 commits into from
Nov 19, 2024
Merged

SAK-50696 - LTI Fix Non-Deterministic Unit Tests #13014

merged 2 commits into from
Nov 19, 2024

Conversation

hermya
Copy link
Contributor

@hermya hermya commented Nov 11, 2024

Fixed flaky test cases in the module: ./lti/tsugi-util/

Test-cases details

Sr Test-case Point of failure Fixed by
1 org.tsugi.lti13.GroupServiceTest# testConstructor The bean of GroupService is converted to Json, and its string representation is asserted against expected value. JSON conversion is non-deterministic, and the order of attributes can be different on different jvms, resulting in different string representations Converting the Json string to a Map and Asserting Map-equality, as it doesn't require matching key-ordering
2 org.tsugi.lti13.LTI13KeySetTest# testKeySets In class org.tsugi.lti13.LTI13KeySetUtil at Line:60, rsaKey is being converted to a json-string, used further in a JsonObject. As rsaKey is a bean, its JSON conversion can result in non-deterministic string Same reason as 1 Converting the Json string to a Map and Asserting Map-equality, and a flexible contains check for asserting goodness of keySetJSON
3 org.tsugi.lti13.LTI13NimbusTest# testRSAFromString, testRSAPaulGray Same reason as 2 for both test-cases Adding a flexible contains check for asserting goodness of keySetJSON
4 org.tsugi.lti13.LTI13ObjectTes# testTwo, testThree Same reason as 1 for both test-cases Same fix as 1 for both test-cases

Fixed using NonDex

Suggested command to check all flaky tests :

mvn edu.illinois:nondex-maven-plugin:2.1.7:nondex

For the particular test class:

mvn -pl ./lti/tsugi-util edu.illinois:nondex-maven-plugin:2.1.7:nondex \
-DnondexMode=ONE \
-DnondexRuns=5  \
-Dtest=org.tsugi.lti13.GroupServiceTest#testConstructor

For more information: https://github.com/TestingResearchIllinois/NonDex

@hermya hermya force-pushed the fix-flaky branch 3 times, most recently from a3745d0 to 31e6678 Compare November 11, 2024 18:03
@hermya hermya mentioned this pull request Nov 11, 2024
Merged
@csev
Copy link
Contributor

csev commented Nov 12, 2024

@hermya Thanks for the patch - it looks good to me. We need to file JIRA in our issue tracker and have you fill out a Contribution Agreement. If your intent is to keep working on Sakai it would be worthwhile to go through all this effort - but if this is a one-time patch, I can take this fix over for you and do all the necessary stuff to get this into the code base. Sorry about the paperwork - open source projects need to do this.

@hermya
Copy link
Contributor Author

hermya commented Nov 12, 2024 •
edited
Loading

Hello @csev,
I'll be happy to fill out the CLA and continue to contribute in future too! Can you please suggest how to do that? I tried logging in on Jira but it shows that I don't have access to create a ticket

@ottenhoff
Copy link
Contributor

I believe there is a form linked from this page to create an individual CLA: https://form.jotform.com/60435795225156

@hermya
Copy link
Contributor Author

hermya commented Nov 12, 2024

Found it and submitted, thanks @ottenhoff!

@ottenhoff ottenhoff requested a review from csev November 15, 2024 17:34
@csev
Copy link
Contributor

csev commented Nov 16, 2024

@ottenhoff - The technical bits look good to me. Once this is merged, I want to apply the approach to some new tests I just wrote this weekend :)

@hermya
Copy link
Contributor Author

hermya commented Nov 17, 2024

Hello All!
I just recently obtained the CLA, is there any other step I must perform?

@csev
Copy link
Contributor

csev commented Nov 18, 2024

I think the one last step it so make a JIRA - @ottenhoff - WDYT?

@ottenhoff
Copy link
Contributor

Agree and let the tests finish running!

@hermya
Copy link
Contributor Author

hermya commented Nov 19, 2024

Hello!
I've been trying to log in to sakai's jira using my email address that I used to create the Apereo license. Even after a couple days , I still can't get access to create a SAK-* ticket. I've mailed a person who responded regarding cla acceptance, but they haven't responded yet. Should I wait for longer (If I am supposed to create the ticket)? Please let me know if I am missing some steps

@csev csev changed the title Fixed 6 flaky tests caused by unordered non-deterministic data-structures in lti/tsugi-util SAK-50696 - LTI Fix Non-Deterministic Unit Tests Nov 19, 2024
@csev csev removed the cla? label Nov 19, 2024
@csev
Copy link
Contributor

csev commented Nov 19, 2024

I made a JIRA and edited the title and the CLA is applied for - so I am merging this. Thanks.

@csev csev merged commit 591047c into sakaiproject:master Nov 19, 2024
5 checks passed
@ottenhoff
Copy link
Contributor

Please try to create a new account at sakaiproject.atlassian.net, login via Google, or send your email address to ottenhoff at longsight dot com

@hermya hermya mentioned this pull request Nov 22, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@csev csev csev approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hermya @csev @ottenhoff @kunaljaykam